Stop using an introduction node in blinded message paths#4647
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
|
||
| // Prefer using non-Tor nodes with the most channels as the introduction node. | ||
| peer_info.sort_unstable_by(|(_, a_tor_only, a_channels), (_, b_tor_only, b_channels)| { | ||
| a_tor_only.cmp(b_tor_only).then(a_channels.cmp(b_channels).reverse()) |
There was a problem hiding this comment.
Similarly, the Tor-only component of this sort is now effectively dead — since the Tor-only filter above is a no-op (see comment there), Tor-only nodes do still get sorted to the back, but they're never excluded. If excluding Tor-only nodes for unannounced recipients isn't intended, the is_tor_only tracking and sorting could be cleaned up for clarity (just sort by channel count).
There was a problem hiding this comment.
This isn't true. If we have more than MAX_PATHS peers and are unannounced the sorting here will result in dropping the tor-only peers.
|
I've thoroughly reviewed the entire PR diff, checked all the changed code paths, verified removed imports and functions are truly unused, and confirmed the logic in both the announced and unannounced recipient branches is correct. My prior review comments (dead Tor-only filter and misleading comment in the unannounced branch) were addressed in a subsequent commit. No new issues found. |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
| } else { | ||
| vec![] | ||
| }; |
There was a problem hiding this comment.
Given we can return more than one path, couldn't we return the 1-hop path (plus dummy hops) in addition to the multi-hop paths? Senders would try all paths, or at least retry over different paths if they don't get a response over the paths requiring a direct connection with an LND node.
There was a problem hiding this comment.
The problem is the BOLT 12 offer creation stuff will pick one path at random (well, currently just the first path, but I'm not sure we want to rely on that at the messagerouter level). Doing this would indeed give us back some privacy for other usecases, but I'm not sure we care as much about hose vs BOLT 12 offers :/.
lnd is preparing to ship a release with opt-in onion messages without support for forwarding onion messages from non-channel peers. This breaks the common BOLT 12 OM flow today where we direct-connect to the blinded path introduction point and send the `invoice_request` without a channel. For CLN it turns out this is fine as they never select a peer for their introduction point at all. However, for LDK this would break existing nodes as nodes might now pick an lnd peer as an introduction node but it won't forward the onion message. For now, we just drop the separate introduction point selection and just always use ourselves as an introduction point (assuming we're an announced node). This should also have the side-effect of making offers marginally more robust, which may be worth it, even if it sucks to drop any pretense of privacy.
39e42bc to
8c08a30
Compare
|
Squashed without further changes. |
|
|
|
I am wondering if for the |
lnd is preparing to ship a release with opt-in onion messages without support for forwarding onion messages from non-channel peers. This breaks the common BOLT 12 OM flow today where we direct-connect to the blinded path introduction point and send the
invoice_requestwithout a channel. For CLN it turns out this is fine as they never select a peer for their introduction point at all. However, for LDK this would break existing nodes as nodes might now pick an lnd peer as an introduction node but it won't forward the onion message.For now, we just drop the separate introduction point selection and just always use ourselves as an introduction point (assuming we're an announced node).
This should also have the side-effect of making offers marginally more robust, which may be worth it, even if it sucks to drop any pretense of privacy.